-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: remove standalone #18157
fix: remove standalone #18157
Conversation
a24bfbc
to
fc41151
Compare
fc41151
to
e83a9b8
Compare
Codecov Report
@@ Coverage Diff @@
## master #18157 +/- ##
==========================================
- Coverage 65.95% 65.87% -0.08%
==========================================
Files 1584 1591 +7
Lines 62063 62409 +346
Branches 6273 6283 +10
==========================================
+ Hits 40934 41114 +180
- Misses 19508 19674 +166
Partials 1621 1621
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up FEATURE_ALERT_REPORTS=True |
@AAfghahi Ephemeral environment spinning up at http://35.88.157.30:8080. Credentials are |
b702980
to
43b26ce
Compare
43b26ce
to
565e2c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! The signature of the function has incorrect types (there should be no Optional
), but I also thought of a way of making the function more generic which might be good.
superset/utils/urls.py
Outdated
@@ -33,3 +33,12 @@ def headless_url(path: str, user_friendly: bool = False) -> str: | |||
def get_url_path(view: str, user_friendly: bool = False, **kwargs: Any) -> str: | |||
with current_app.test_request_context(): | |||
return headless_url(url_for(view, **kwargs), user_friendly=user_friendly) | |||
|
|||
|
|||
def get_screenshot_explorelink(url: Optional[str]) -> Optional[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a docstring to the function explaining what it does? As for types, it's better to have the signature be (url: str) -> str:
, otherwise you need to handle the case where url
is None
.
One thing I was thinking is that since this function is currently very specific we could make it a bit more generic, so it has utility outside of your use case. Eg:
def modify_url_query(url: str, **kwargs: Any) -> str:
"""
Replace or add parameters to a URL.
"""
parts = list(urllib.parse.urlsplit(url))
params = urllib.parse.parse_qs(parts[3])
for k, v in kwargs.items():
if not isinstance(v, list):
v = [v]
params[k] = v
parts[3] = "&".join(f"{k}={urllib.parse.quote(v[0])}" for k, v in params.items())
return urllib.parse.urlunsplit(parts)
Then you can call it for your use case as:
url = modify_url_query(self._content.url, standalone='0')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I think that is a cool generic function, thank you! When I had (url: string) -> string
I got a type error, and since i was rushing against the cherry deadline I made it optional, will change back and think it through. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you had an error with (url: str) -> str
it means that in some place the function was potentially being called with a non-string value (probably None
, since you fixed it by adding Optional
). Since your original function did not handle the case when the argument was not a string, this means that at some point the code would break.
In other words, if your function has a signature (url: Optional[str])
it should be able to handle being passed url=None
. But more importantly, the types in the signature should make sense — would we ever want to pass None
as a URL to a function that changes parameters in the URL?
The proper way to fix the type checking is not relaxing the signature, but adding guards to the code:
if url is not None:
explore_url = get_screenshot_explorelink(url)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of adding an exception into the modify_url_query function, but this feels like it would look better in the email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we do end up using the URL in the email body later I was thinking that we could do this:
url = ""
if self._content.url is not None:
url = modify_url_query(self._content.url, standalone="0")
The code before was using self._content.url into the email. so if we were invoking None, would it be the same as invoking an empty string?
2760891
to
f0b74be
Compare
f0b74be
to
89d7e58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
705b344
to
8c7e2f8
Compare
Ephemeral environment shutdown and build artifacts deleted. |
* removed standalone * Update tests/integration_tests/reports/commands_tests.py * changed standalone * added tests * made function more generic * Update superset/reports/notifications/email.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
* removed standalone * Update tests/integration_tests/reports/commands_tests.py * changed standalone * added tests * made function more generic * Update superset/reports/notifications/email.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
* removed standalone * Update tests/integration_tests/reports/commands_tests.py * changed standalone * added tests * made function more generic * Update superset/reports/notifications/email.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
SUMMARY
Having standalone 3 in the url prevented tabbed dashboards from showing in a report.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION